Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[make:voter] generate type hints #853

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

jrushlow
Copy link
Collaborator

starting in Symfony 5.0 the abstract voter methods introduced type hints (symfony/symfony@8c46b95). We now conditionally generate the string type hint for the $attribute argument.

Minor cleanup of the maker itself.

@@ -8,15 +8,15 @@

class <?= $class_name ?> extends Voter
{
protected function supports($attribute, $subject)
protected function supports(<?= $use_type_hints ? 'string ' : null ?>$attribute, $subject): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have this conversation on every PR :p. Would generating the types break if I were using Symfony 4.4, where the parent class does NOT have those hints? If so, then we should do this - but only if we're on Symfony 5.

Copy link
Collaborator Author

@jrushlow jrushlow Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - i keep forgetting how this works myself, but if the abstract does not declare a return type, the child is able to declare one. For arguments it would break iirc...

// https://www.php.net/manual/en/language.types.declarations.php

Note:

When overriding a parent method, the child's method must match any return type declaration on the parent. If the parent doesn't define a return type, then the child method may do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then we should do this - but only if we're on Symfony 5.

The new $use_type_hints var checks if we're using symfony 5 or greater (double check my comparison operator :D) for the arg. But this check isnt needed for the return type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - i keep forgetting how this works myself, but if the abstract does not declare a return type, the child is able to declare one. For arguments it would break iirc...

Ah yes. It's legal to make a return type "more restrictive" than your parent, but it is NOT legal to make an argument type stricter (which would now mean you accept LESS input)

@weaverryan weaverryan force-pushed the refactor/make-voter branch from 2bb10ff to 95980dc Compare March 30, 2021 12:51
@weaverryan
Copy link
Member

Thanks Jesse!

@weaverryan weaverryan merged commit b718c4f into symfony:main Mar 30, 2021
@jrushlow jrushlow deleted the refactor/make-voter branch May 3, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants